-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add discover_imports
in conf, don't collect imported classes named Test* closes #12749`
#12810
base: main
Are you sure you want to change the base?
Conversation
- Allows a user to define whether or not pytest should treat imported (but not in testpaths) as test classes. - Imagine a class is called TestFoo defined in src/ dir, when discover_imports is disabled, TestFoo is not treated as a test class.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this started
We have to do some considerations for backwards compatibility and type safety
Also I'm not yet sure if the selection approach is generally intuitive (it solving your usecase nicely
parser.addini( | ||
"discover_imports", | ||
"Whether to discover tests in imported modules outside `testpaths`", | ||
default=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to start with a default of true for backwards compatibility
Alternatively none with a informative warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it helps, I'd be happy to argue that the current behavior was a substantial surprise to me, so that defaulting to False would count as removing a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it helps, I'd be happy to argue that the current behavior was a substantial surprise to me, so that defaulting to False would count as removing a bug.
I understand where you are coming from and I agree this would be the reasonable default if pytest was being born today... but unfortunately in this case I suspect there are test suites which rely on this behavior, so we really should be conservative here.
@@ -741,6 +741,12 @@ def newinstance(self): | |||
return self.obj() | |||
|
|||
def collect(self) -> Iterable[nodes.Item | nodes.Collector]: | |||
if self.config.getini("discover_imports") == ("false" or False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should ensure this is always a boolean or parsed somewhere else to ensure Separation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think changing the default value for the option to False
and changing this to if self.config.getinit("discover_imports"):
should be enough.
@RonnyPfannschmidt Hi! Thanks for reviewing :) To be clear if I would set True to default (as you suggested) this would mean that the base case is that a class like "Testament" in a seperate folder would make pytest error out (see #12749). Then a user would opt in (set to false) to make sure that the discovery does not happen for those classes. Your suggestion seems more reasonable I think. I saw some checks failed, which didn't happen locally for some reason. |
@@ -78,6 +78,11 @@ def pytest_addoption(parser: Parser) -> None: | |||
type="args", | |||
default=[], | |||
) | |||
parser.addini( | |||
"discover_imports", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea that discover_imports
is really only active if 'testpaths' is also defined in pytest.ini
?
@@ -0,0 +1,3 @@ | |||
Add :confval:`discover_imports`, when disabled (default) will make sure to not consider classes which are imported by a test file and starts with Test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, changelog entry.
We also need to add documentation for that confval in reference.rst
.
I'm not sure about the name too, but at the moment I don't have another suggestion (however we really should strive to find a more appropriate name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
treat_imports_as_tests?
imports_are_tests?
crawl_imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per my comment, perhaps we should focus on being more "strict" about what we collect from modules.
collect_?something? = "all"/"local-only"
But we should wait to see what other maintainers think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, now after a some hours (more than id like to admit) I now have something that works. Both for functions and classes. Let me know how we wanna handle the toggle of the feature. Have a good weekend!
I also wonder if we should be using a more general approach, and not only classes: shouldn't we also account for |
Thought about this some more: I think this should actually be expanded to the general idea/feature of "pytest will only collect objects from modules if they are defined in that module" (by inspecting its |
Another thing that occurred to me: @pytest-dev/core how do you feel about this feature, should we go ahead with it or are there other concerns to discuss? |
I'm completely unsurprised that classes or functions imported into a test file are collected, but I'm also obviously atypical of pytest users, and so having a setting for this could make sense. If we do, I think the setting should be named
Agree with all of this, Finally, I'm concerned that this opens up another avenue for the worst kind of testing bug, where you think you have tests but they can't actually fail (because they're not collected!) and so the whole exercise is silently useless. I'm unsure whether the benefits are worth this risk, given that there are plenty of other workarounds based on our existing features - including collection patterns, plugins, etc. |
closes #12749